fix(scripts): count subdirectory-only dirs as non-empty in PowerShell (parity with bash)#3137
Open
jawwad-ali wants to merge 2 commits into
Open
fix(scripts): count subdirectory-only dirs as non-empty in PowerShell (parity with bash)#3137jawwad-ali wants to merge 2 commits into
jawwad-ali wants to merge 2 commits into
Conversation
Test-DirHasFiles (the documented PowerShell twin of bash check_dir) tested
non-emptiness with `Get-ChildItem | Where-Object { -not $_.PSIsContainer }`,
counting only top-level FILES and ignoring subdirectories. Bash check_dir
(`-n $(ls -A ...)`) and the PowerShell JSON-path contracts checks
(check-prerequisites.ps1 / setup-tasks.ps1, no PSIsContainer filter) both
count ANY entry. So a contracts/ directory whose only contents are
subdirectories (e.g. contracts/v1/openapi.yaml) was reported present by
bash, by bash JSON, and by PowerShell JSON, but [FAIL]/absent by PowerShell
text mode — the lone outlier.
Drop the PSIsContainer filter so Test-DirHasFiles counts any entry, matching
the other three code paths.
Add bash + PowerShell parity tests asserting a subdir-only contracts/ dir is
reported non-empty in both shells.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a parity bug between the bash and PowerShell task scripts by making PowerShell’s Test-DirHasFiles treat a directory containing only subdirectories as “non-empty,” aligning behavior with the other prerequisite-check code paths.
Changes:
- Update
Test-DirHasFilesinscripts/powershell/common.ps1to treat any directory entry (including subdirectories) as non-empty. - Add regression tests to assert bash
check_dirand PowerShellTest-DirHasFilesboth treat “subdir-only” directories as non-empty.
Show a summary per file
| File | Description |
|---|---|
scripts/powershell/common.ps1 |
Removes the subdirectory-excluding filter so non-emptiness counts any child entry. |
tests/test_setup_tasks.py |
Adds bash + PowerShell parity tests for subdirectory-only directory layouts. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
Address review feedback on Test-DirHasFiles parity fix: - Reword the common.ps1 comment so it no longer claims exact `ls -A` parity (Get-ChildItem omits hidden entries without -Force); it now points at the in-repo PowerShell JSON contracts checks as the matching reference and keeps the subdir-only-is-non-empty rationale. - Rename test_test_dir_has_files_ps_... -> test_dir_has_files_ps_... to drop the doubled 'test_' prefix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Thanks for the review! Addressed in 6810368:
AI disclosure: prepared with Claude Code (Claude Opus 4.8) under my direction; I reviewed the diff before pushing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Test-DirHasFilesinscripts/powershell/common.ps1(the documented PowerShell twin of bashcheck_dir) tested directory non-emptiness with:The
Where-Object { -not $_.PSIsContainer }filter counts only top-level files and ignores subdirectories. But the three sibling code paths all count any entry:check_dir(common.sh):[[ -d "$1" && -n $(ls -A "$1") ]]check-prerequisites.ps1,setup-tasks.ps1):Get-ChildItem ... | Select-Object -First 1(no filter)So a
contracts/directory whose only contents are subdirectories (e.g. a versioned/grouped layoutcontracts/v1/openapi.yaml) was reported present by bash, by bash JSON, and by PowerShell JSON — but[FAIL]/absent by PowerShell text mode.Test-DirHasFilesis the lone outlier, and it's internally inconsistent with the JSON path in its own files.Fix
Drop the
PSIsContainerfilter soTest-DirHasFilescounts any entry, matching the other three paths (one-line change).Testing
Tested locally with
uv run specify --help(exit 0)Ran existing tests with
uv sync && uv run pytestuvx ruff checkclean.uv run pytest tests/test_setup_tasks.py— 10 passed, 16 skipped (no regressions; bash + pwsh-only tests skip on my host).Verified directly with PowerShell: before, a subdir-only
contracts/→[FAIL]; after →[OK]; a genuinely empty dir and a missing dir still →[FAIL].New parity tests in
tests/test_setup_tasks.py(bashcheck_dir+ PowerShellTest-DirHasFiles) assert a subdir-onlycontracts/is reported non-empty in both shells. The PowerShell test fails onmain([FAIL] contracts/) and passes with this fix.AI Disclosure
Found and fixed with Claude Code (Claude Opus 4.8) under my direction. AI located the divergence across the four code paths; I reproduced the
[FAIL]vs present discrepancy under PowerShell, confirmed the one-line fix restores parity (and that empty/missing dirs still report[FAIL]), and reviewed the diff before submitting. I'll disclose if any review responses are AI-assisted as well.